[TNTP-2109] Switch to safe mode on vshard rebalance #467

ita-sammann · 2025-12-05T10:59:56Z

Implement "safe" mode to prevent writing data to wrong replicaset when vshard rebalance is in progress.
Auto switch to safe mode when rebalance process starts.
Manual return to fast mode.

I didn't forget about

Tests
Changelog
Documentation

Co-authored-by: Satbek Turganbayev <s.turganbayev@corp.mail.ru>

In fast mode ref/unref do nothing. In safe mode they call bucket_refro,bucket_refrw, bucket_unrefo,bucket_unrefrw. Also added ref error handle from storages. On ref error router will reset bucket, change replicaset for single operations and retry request.

* add access to _bucket space in privillage tests * fix select_readview_test * enable double buckets test * add safe_mode_disable for cartirdge reload test: test_storage * add safe_mode_disable for test_any_vshard_call_timeout

crud/common/sharding/bucket_ref_unref.lua

crud/common/call.lua

crud/common/rebalance.lua

Satbek · 2025-12-08T07:52:44Z

test/integration/double_buckets_test.lua

+pgroup_duplicates.test_duplicates = function(g)
+    t.skip_if(
+        not (
+            utils.tarantool_version_at_least(3, 1) and (g.params.backend == "config")


todo: implement test for other backend types. 2.11 and cartridge

test/integration/double_buckets_test.lua

test/helper.lua

vakhov

Thanks for the PR! I’ve gone through the changes and left a few small questions.

crud/common/call.lua

crud/common/rebalance.lua

crud/common/sharding/bucket_ref_unref.lua

Co-authored-by: Satbek Turganbayev <s.turganbayev@corp.mail.ru>

Serpentian · 2025-12-11T09:52:22Z

crud.lua

 local utils = require('crud.common.utils')
 local stats = require('crud.stats')
 local readview = require('crud.readview')
 local schema = require('crud.schema')


Guys, the commits are absolutely chaotic, this is not the way, we develop open-source modules. We should rebase that branch in master to make the commit history linear, and not just merge it. So, all commits like Minor changes according to review comments will be visible to our users. You can check out, how Georgy Moiseev did it: proper commits, proper commit messages, tests in every commit (e.g. 8d7cae0).

Now, I'm forced to review more than 1k lines in one step, which is very inconvenient and increases the chanse of missed bugs. And our users won't be able to check the individual commits, if they want to. Of course, it's up to you, since I'm not the maintainer of that module, but it's just not nice to develop and merge code like that.

IMHO, the features should be properly split between commits, every commit must include the associated tests, proper commit messages and mentioning of the #448 ticket. Of course, refactoring or code moving should be in the separate commits. Between commits all of the test must pass

Guys, the commits are absolutely chaotic

Of course these commits will not go to master, I will re-split them before merging the PR.

Now, I'm forced to review more than 1k lines in one step

My bad. Never thought of this PR as of multiple features that can be reviewed separately.

Serpentian · 2025-12-11T09:56:38Z

crud.lua

+
+-- @refer rebalance.router_cache_length
+-- @function router_cache_length
+crud.rebalance.router_cache_length = rebalance.router.cache_length


Why do we export router_cache_length and router_cache_last_clear_ts to public API? I understand that for router_cache_clear, which is supposed to be called by the TCM/cartridge/ATE, but I don't understand, why getters are needed

I think they will be useful in TCM/Cartridge to indicate that cache has actually been cleared and when did it happen.

Serpentian · 2025-12-11T10:08:06Z

crud.lua

 function crud.init_router()
-   rawset(_G, 'crud', crud)
+    rawset(_G, 'crud', crud)
+    rebalance.metrics.enable_router_metrics()


Looks like a crutch. Will it be disabled, when user disables metrics? It seems the whole metrics part should be done in the metrics_registry

Serpentian · 2025-12-11T10:09:30Z

crud/common/rebalance.lua

+    safe_mode = false,
+    safe_mode_enable_hooks = {},
+    safe_mode_disable_hooks = {},
+    _router_cache_last_clear_ts = fiber.time()


Why do we initialize the timestamp with current time on module load? It may lead to bugs, we didn't really cleared the router`s cache on module load

Serpentian · 2025-12-11T10:14:34Z

crud/common/rebalance.lua

+    end)
+end
+
+M.init = rebalance_init


Nit: let's make the code consistent with all other modules, it seems we tend to make the variable with proper name and assign methods to it .

crud/crud/stats/local_registry.lua

Line 39 in c52a6f5

function registry.init(opts)

Serpentian · 2025-12-11T14:19:39Z

crud/common/sharding/bucket_ref_unref.lua

+        if not ref_ok then
+
+            table.insert(bucket_ref_errs, bucket_refrw_err.bucket_ref_errs[1])
+            goto continue


Why do we continue on error and then just unref all of them? We should go to unref stright away

Serpentian · 2025-12-11T14:22:01Z

crud/common/sharding/bucket_ref_unref.lua

+        set_fast_mode()
+    end
+
+    if not hooks_registered then


You won't need it, if internal.trigger will be used

Serpentian · 2025-12-11T14:23:33Z

crud/common/sharding/bucket_ref_unref.lua

+    end
+end
+
+set_fast_mode()


Hmm, and what if rebalance module is currently in the safe mode, then the trigger won't work and the bucket_ref_unref will remain in the fast. Or am I missing smth?

Serpentian · 2025-12-11T14:26:11Z

crud/common/call.lua

+            end
+        end
+
+        if redirect_replicaset ~= nil then


Nit: why can't we just change the replicaset variable, seems redirect_replicaset is not needed here

Serpentian · 2025-12-11T14:29:17Z

crud/insert.lua

-        noreturn = opts.noreturn,
-        fetch_latest_metadata = opts.fetch_latest_metadata,
-    })
+    local function make_insert()


Can't we just make function wrappers for single bucket and multiple ones? Why do we always repeat the same code

ita-sammann and others added 5 commits December 5, 2025 13:42

[TNTP-2109] Switch to safe mode on vshard rebalance

2f630af

[TNTP-2109] Add safe mode with bucket_ref/unref

9b626bb

[TNTP-2109] Add tests for safe mode

abbcfbd

Co-authored-by: Satbek Turganbayev <s.turganbayev@corp.mail.ru>

Fix tests after fast/safe mode changes

8ea36f9

* add access to _bucket space in privillage tests * fix select_readview_test * enable double buckets test * add safe_mode_disable for cartirdge reload test: test_storage * add safe_mode_disable for test_any_vshard_call_timeout

ita-sammann mentioned this pull request Dec 5, 2025

TNTP-2109: call bucket_ref/bucket_unref on crud operations #463

Closed

3 tasks

ita-sammann requested a review from Satbek December 5, 2025 11:31

ita-sammann marked this pull request as ready for review December 5, 2025 13:38

ita-sammann requested review from DifferentialOrange, a1div0, p0rtale and vakhov December 5, 2025 13:38

Allow some fast methods to finish after rebalance started

a8352f2

Satbek requested changes Dec 8, 2025

View reviewed changes

vakhov reviewed Dec 8, 2025

View reviewed changes

crud/common/call.lua Outdated Show resolved Hide resolved

crud/common/rebalance.lua Show resolved Hide resolved

crud/common/sharding/bucket_ref_unref.lua Outdated Show resolved Hide resolved

crud/common/sharding/bucket_ref_unref.lua Show resolved Hide resolved

Minor changes according to review comments

bb11b86

ita-sammann force-pushed the TNTP-2109-rebalance-safe-mode branch from 4a8e4d2 to bb11b86 Compare December 9, 2025 11:28

Satbek and others added 4 commits December 10, 2025 12:22

make_bucket_ref_err: call BucketRefError only once

d937de4

fix double_buckets_not_applied test, now works for get

c7a913e

rm manual fast mode enable in select_readview_test

b8cf51d

Fix typo in comment

b552571

Co-authored-by: Satbek Turganbayev <s.turganbayev@corp.mail.ru>

ita-sammann requested review from Satbek, Serpentian and vakhov December 10, 2025 08:19

vakhov approved these changes Dec 10, 2025

View reviewed changes

ita-sammann self-assigned this Dec 10, 2025

ita-sammann added the do not merge Not ready to be merged label Dec 10, 2025

Satbek approved these changes Dec 10, 2025

View reviewed changes

Serpentian suggested changes Dec 11, 2025

View reviewed changes

[TNTP-2109] Switch to safe mode on vshard rebalance #467

Are you sure you want to change the base?

[TNTP-2109] Switch to safe mode on vshard rebalance #467

Conversation

ita-sammann commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vakhov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ita-sammann Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ita-sammann commented Dec 5, 2025 •

edited

Loading

ita-sammann Dec 12, 2025 •

edited

Loading